Codecov Report

Attention: Patch coverage is 94.31818% with 5 lines in your changes missing coverage. Please review.

What this PR calls _select_algorithm was called algorithm_or_select_algorithm in #19, that is at least a more descriptive name for what it is doing.

EDIT: So to clarify, the proposal would be to change select_algorithm -> algorithm_or_select_algorithm or maybe_select_algorithm and default_algorithm -> select_algorithm.

If we decide on this, can you also look at exporting default_algorithm, since that would be the main point to overload, and check if the docs need updating for this new approach?

What do you think of making it public but not exporting it for now, indicating it is more of a syntax for overloading rather than using externally? (EDIT: I made default_algorithm and select_algorithm public for now but I'm happy to change that.)

Currently, if you pass an algorithm type or Symbol as the alg keyword argument, select_algorithm and therefore the factorizations are not type stable. I thought constant propagation would handle that but I guess not. I tried playing around with @inline but it didn't seem to help.

I don't have any strong feelings about public or exported in this case. Given that our use-cases are mostly to use this as a low-level library to build on, I don't think it matters too much. As you noticed public doesn't work in 1.10, I think the recommended way of resolving that is using Compat.jl to handle this instead of doing that manually.

I'm a bit more worried about the type stability though.
Was this type-stable before?
There's also Base.@constprop :aggressive as an option, or explicitly Val-ing some arguments in the internal implementations.

I don't have any strong feelings about public or exported in this case. Given that our use-cases are mostly to use this as a low-level library to build on, I don't think it matters too much. As you noticed public doesn't work in 1.10, I think the recommended way of resolving that is using Compat.jl to handle this instead of doing that manually.

There are long debates about that: https://discourse.julialang.org/t/is-compat-jl-worth-it-for-the-public-keyword/119041, the approach I use here is the one recommended in the Julia docs: https://docs.julialang.org/en/v1.12-dev/manual/modules/#Export-lists. Some people are weary of Compat.jl because it causes a lot of invalidations and uses type piracy, so if there is a simple way to avoid it it seems better to do so, but I'm fine either way.

I'm a bit more worried about the type stability though. Was this type-stable before? There's also Base.@constprop :aggressive as an option, or explicitly Val-ing some arguments in the internal implementations.

I doubt it was type stable before but I can double check. I also tried Base.@constprop :aggressive but that didn't seem to help, though I can explore that a bit more, and I can also look into if some use of Val can help.

I think basically what we are doing is analogous to this minimal example:

julia> f(; x) = Val{x}()
f (generic function with 1 method)

julia> @code_warntype f(; x=:x)
MethodInstance for Core.kwcall(::@NamedTuple{x::Symbol}, ::typeof(f))
  from kwcall(::NamedTuple, ::typeof(f)) @ Main REPL[56]:1
Arguments
  _::Core.Const(Core.kwcall)
  @_2::@NamedTuple{x::Symbol}
  @_3::Core.Const(Main.f)
Locals
  @_4::Symbol
  x::Symbol
Body::Val
1 ─       Core.NewvarNode(:(@_4))
│   %2  = Core.isdefined(@_2, :x)::Core.Const(true)
└──       goto #3 if not %2
2 ─       (@_4 = Core.getfield(@_2, :x))
└──       goto #4
3 ─       Core.Const(:(Core.UndefKeywordError(:x)))
└──       Core.Const(:(@_4 = Core.throw(%6)))
4 ┄ %8  = @_4::Symbol
│         (x = %8)
│   %10 = Base.keys(@_2)::Core.Const((:x,))
│   %11 = (:x,)::Core.Const((:x,))
│   %12 = Base.diff_names(%10, %11)::Core.Const(())
│   %13 = Base.isempty(%12)::Core.Const(true)
└──       goto #6 if not %13
5 ─       goto #7
6 ─       Core.Const(:(Base.kwerr(@_2, @_3)))
7 ┄ %17 = Main.:(var"#f#1")::Core.Const(Main.var"#f#1")
│   %18 = x::Symbol
│   %19 = (%17)(%18, @_3)::Val
└──       return %19

julia> @code_warntype (() -> f(; x=:x))()
MethodInstance for (::var"#5#6")()
  from (::var"#5#6")() @ Main REPL[61]:1
Arguments
  #self#::Core.Const(var"#5#6"())
Body::Val{:x}
1 ─ %1 = (:x,)::Core.Const((:x,))
│   %2 = Core.apply_type(Core.NamedTuple, %1)::Core.Const(NamedTuple{(:x,)})
│   %3 = (:x,)::Core.Const((:x,))
│   %4 = (%2)(%3)::Core.Const((x = :x,))
│   %5 = Core.kwcall(%4, Main.f)::Core.Const(Val{:x}())
└──      return %5

julia> using TestExtras

julia> @constinferred f(; x=:x)
Val{:x}()

julia> @constinferred f(; x=Symbol("x"))
Test Failed at REPL[75]:1
  Expression: @constinferred f(; x = Symbol("x"))
   Evaluated: Val{:x} != Val

ERROR: There was an error during testing

so it seems like at least in principle it should be inferrable.

Regarding the type stability, I just remembered/realized that this change has an additional side-effect that might affect this: before we didn't have two layers (select_algorithm and default_algorithm), and as a result the implementations were written directly in terms of select_algorithm(::typeof(f), ...), while now there are a bunch of definitions for select_algorithm(f, ...). Given that Julia doesn't specialize on functions by default, I'm wondering if this might be affecting the outcomes. In other words, I'm wondering if it's not better to write select_algorithm(f::F, ...) where {F} if we want to have these two layers of functions in there.

Ok, I've solved most type instability issues and added more tests. I had to rely both on forcing method specialization and Base.@constprop :aggressive as @lkdvos suggested.

Oddly, the one case that still isn't type stable is specifying alg as a type, i.e. svd_compact!(randn(2, 2); alg=LAPACK_QRIteration). Any suggestions with that would be welcome...

This is ready for review. All type stability issues are fixed, except for one issue in Julia 1.10, I would vote for letting that slide (as opposed to making the code more complicated to fix it) but please let me know if you want me to look into that.

In the latest, I was able to avoid all uses of Base.@constprop :aggressive. @lkdvos pointed out to me that I was using @constinferred wrong and should have been interpolating the alg input in the tests.

Thanks again for the careful review.

Should we register v0.2 after #25 gets merged?

Yes that sounds good to me. I went through #25 and do not have any comments.